Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

events: WS protobuf messages should be binary #19232

Merged
merged 6 commits into from
Feb 17, 2023
Merged

events: WS protobuf messages should be binary #19232

merged 6 commits into from
Feb 17, 2023

Conversation

swenson
Copy link
Contributor

@swenson swenson commented Feb 16, 2023

The WebSockets spec states that text messages must be valid UTF-8 encoded strings, which protobuf messages virtually never are.

This instead correctly sends the protobuf events as binary messages.

The [WebSockets spec](https://www.rfc-editor.org/rfc/rfc6455) states
that text messages must be valid UTF-8 encoded strings, which protobuf
messages virtually never are.

This instead correctly sends the protobuf events as binary messages.
Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one off-topic question

http/events.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@swenson
Copy link
Contributor Author

swenson commented Feb 17, 2023

Thanks!

@swenson swenson merged commit 4944581 into main Feb 17, 2023
@swenson swenson deleted the websocket-binary branch February 17, 2023 19:38
swenson pushed a commit that referenced this pull request Feb 17, 2023
The [WebSockets spec](https://www.rfc-editor.org/rfc/rfc6455) states
that text messages must be valid UTF-8 encoded strings, which protobuf
messages virtually never are. This now correctly sends the protobuf events
as binary messages.

We change the format to correspond to CloudEvents, as originally intended,
and remove a redundant timestamp and newline.

We also bump the eventlogger to fix a race condition that this code triggers.
swenson pushed a commit that referenced this pull request Feb 17, 2023
The [WebSockets spec](https://www.rfc-editor.org/rfc/rfc6455) states
that text messages must be valid UTF-8 encoded strings, which protobuf
messages virtually never are. This now correctly sends the protobuf events
as binary messages.

We change the format to correspond to CloudEvents, as originally intended,
and remove a redundant timestamp and newline.

We also bump the eventlogger to fix a race condition that this code triggers.

Co-authored-by: Christopher Swenson <christopher.swenson@hashicorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants